-
Notifications
You must be signed in to change notification settings - Fork 2.5k
[HUDI-3528] Fix String convert issue and overwrite putAll method in Typed… #4920
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
7a602cc to
80b76f2
Compare
|
@stayrascal : this does not looks like minor. can you please file a jira and link it. |
|
CC @codope. |
hudi-common/src/test/java/org/apache/hudi/common/properties/TestTypedProperties.java
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/common/config/TypedProperties.java
Show resolved
Hide resolved
|
Thanks @nsivabalan review this PR. I failed to create a Jira card, not sure where I can create the card or I didn't have the permission. Instead, I create an issue here. |
nsivabalan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
codope
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm.
hudi-common/src/main/java/org/apache/hudi/common/config/TypedProperties.java
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/common/config/TypedProperties.java
Show resolved
Hide resolved
…ypedProperties.java (apache#4920)
…ypedProperties.java (apache#4920)
…ypedProperties.java (apache#4920)
…Properties.java
Tips
What is the purpose of the pull request
I'm running Flink with Hudi in Java 11 environment(Even through I know Hudi might only support Java8/9/10), But we cannot change our JDK version. Based on this situation, We meet a NPE problem during
HoodieWriteConfig.getIndexType(), but I test that it works in my local Java 8 environment. It seems that we implemented aTypedPropertiesclass inherit fromjava.util.Propertiesclass, and overwritestringPropertyNamesmethod &putmethod from this PR.There are some difference of
Properties.putAll(Map<?,?> t)between Java 8 and Java 11.Java 8:
TypedPropertieswill use HashTable'sputAllmethod, which will callTypedProperties.putmethod, and the latter will add the incoming keys inTypedProperties.keysfield.Java 11:
PropertiesoverwriteputAllmethod, and delegate the incoming map toConcurrentHashMap. It won't callTypedProperties.putmethod anymore, which lead theTypedProperties.keysfields miss some keys, and then it cause that we miss thehoodie.index.typeconfiguration.So we also need to overwrite
putAllmethod to better suitable high Java version runtime.The exception trace with Java 11 runtime.
Brief change log
putAllmethod inTypedPropertisVerify this pull request
This change added tests and can be verified as follows:
putAllcases in TestTypedProperties.javaCommitter checklist
Has a corresponding JIRA in PR title & commit
Commit message is descriptive of the change
CI is green
Necessary doc changes done or have another open PR
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.